-
Notifications
You must be signed in to change notification settings - Fork 76
[code_builder] Add control-flow support #2135
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
* add `ForLoop`, `ForInLoop`, `WhileLoop` classes and associated builders, as well as relevant tests. * add `ControlBlockVisitor` and `ControlBlockEmitter` classes for control block emitting knowledge. * mix `ControlBlockEmitter` into `DartEmitter` to add general control-block emitting capabilities. * mark `ControlExpression` as internal and stop exporting it. move `ControlExpression` tests to a dedicated file in `tests/specs/code`. *still todo*: support emitting if and try/catch blocks and trees
…` version * rebuild `*.g.dart` files in `lib/src/specs` that were generated with an older version of `built_value` and follow older dart conventions. * bump package version to 4.11.0-wip to reflect changes * dev dependencies: require `source_gen: ^3.0.0` and `build: ^3.0.0` all tests still pass 👍
* update markdown files to follow commonmark spec
* bump version number in changelog
* add `doc/api` (`dart doc` generated docs directory) to `.gitignore`
* recommend dart, spell checker, and markdown linter extensions for vscode
* configure workspace extension settings
* add `ControlTree` class for sharing visitor logic among different control-block tree types * add `Condition` (+ builder) for representing a single if/else condition * add `IfTree` (+ builder) for representing an if/else tree * add helper functions to `IfTree` for easier usage * move `ControlExpression` visitor/emitter logic to `ControlBlockVisitor` and `ControlBlockEmitter`; stop exporting those internal-only classes * add `ControlFlow` extension on `Expression`; move expression control-flow helpers to `ControlFlow` * add `Expression` helpers for creating loops and if trees to `ControlFlow`; support chaining to easily create if trees * add relevant tests
* add `CatchBlock` and `TryCatch` + builders for generating catch clauses and try/catch blocks * add `ControlFlow.rethrow` to support `rethrow` keyword * recommend build_value helper extension for convenience * add relevant tests
* add `Case` (+ builder) for creating cases in switch statements * add `SwitchStatement` (+ builder) for emitting switch statements * add relevant tests still todo: add switch *expressions*
* add `SwitchExpression` (+ builder) for emitting switch expressions * move `ControlFlow.wildcard` to `Expression.wildcard` * add/update relevant tests
* update literal collection emitters to support chaining * add `collectionIf`, `collectionElse`, `collectionFor`, and `collectionForIn` static methods to `ControlFlow` for emitting collection control-flow expressions * remove unnecessary helper function * add relevant tests
|
Woah! My little helper method grew up! Didn't expect to see this when I woke up today! Anyway, I notice (looking at the tests) that the catch blocks have a default name for the exception - i'd honestly just put a wildcard if its empty. We don't always want the value, since we can always rethrow after doing whatever. (plus we dont want code_builder to be the reason for shadowing, right? Who knows what local variables devs make?) Additionally, if neither exception nor stack are specified but the on type is, then it should omit the How you handle switch statements/expressions seems to be pretty good, at least on the surface. The Edit: looking further, it does look like you have those extensions, so it may not be a problem at all. That said, the collection ones seem to be third class citizens. I think that does need improving, because we should effectively have the full power of our conditionals and loops, just with an expression in place of a body. Edit 2: It may be that we're just missing if-case for that actually, though i could be missing something myself. Something to look into. |
|
Thanks for the suggestions!
Fully agree here, updated to fix that. I guess I always just use
Fixed this as well. I knew I forgot something! Totally overlooked this during development.
Same here to some degree, but I couldn't really find a better solution. A
This is also something that somewhat bugs me as well. The way I see it, there are three ways to implement collection control-flow:
If-case is actually supported via using ControlFlow.collectionIf(
condition: ControlFlow.ifCase(
object: // ...
pattern: // ...
),
value: // ...
);Maybe I should add a dedicated helper like |
* replace IfTree/Condition with Conditional and BranchBuilder * move `switch` logic to a dedicated file * add ifThenReturn to Expression
|
Alright, I thought for a little longer on the IfTree/Condition separation and made some changes. First off, I renamed
Updated the tests, changelog and docs to reflect this. Also, I realized that despite mentioning Hopefully these should address the overlap. Now that I'm out of the dev tunnel vision I realize how confusing that was. |
|
|
||
| * Fix confusing mismatch description from `equalsDart`. | ||
| https://github.com/dart-lang/code_builder/issues/293 | ||
| <https://github.com/dart-lang/code_builder/issues/293> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
avoid other changes to this file. it makes it hard to review!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
apologies, that was my formatter as i inadvertently left format on save enabled
PR Health
Breaking changes
|
| Package | Change | Current Version | New Version | Needed Version | Looking good? |
|---|---|---|---|---|---|
| code_builder | Breaking | 4.10.1 | 4.11.0-wip | 5.0.0 Got "4.11.0-wip" expected >= "5.0.0" (breaking changes) |
This check can be disabled by tagging the PR with skip-breaking-check.
Changelog Entry ✔️
| Package | Changed Files |
|---|
Changes to files need to be accounted for in their respective changelogs.
Coverage ⚠️
| File | Coverage |
|---|---|
| pkgs/code_builder/lib/code_builder.dart | 💔 Not covered |
| pkgs/code_builder/lib/src/emitter.dart | 💚 98 % ⬆️ 0 % |
| pkgs/code_builder/lib/src/mixins/control.dart | 💚 100 % |
| pkgs/code_builder/lib/src/specs/control.dart | 💚 97 % |
| pkgs/code_builder/lib/src/specs/control/branches.dart | 💚 100 % |
| pkgs/code_builder/lib/src/specs/control/handling.dart | 💚 100 % |
| pkgs/code_builder/lib/src/specs/control/loops.dart | 💚 100 % |
| pkgs/code_builder/lib/src/specs/control/switch.dart | 💚 100 % |
| pkgs/code_builder/lib/src/specs/expression.dart | 💚 97 % ⬆️ 0 % |
| pkgs/code_builder/lib/src/specs/expression/control.dart | 💚 100 % |
This check for test coverage is informational (issues shown here will not fail the PR).
This check can be disabled by tagging the PR with skip-coverage-check.
API leaks ⚠️
The following packages contain symbols visible in the public API, but not exported by the library. Export these symbols or remove them from your publicly visible API.
| Package | Leaked API symbol | Leaking sources |
|---|---|---|
| code_builder | SpecVisitor | base.dart::Spec::accept::visitor emitter.dart::DartEmitter specs/code.dart::CodeVisitor specs/code.dart::CodeEmitter specs/expression.dart::ExpressionVisitor specs/expression.dart::ExpressionEmitter specs/control.dart::ControlBlockVisitor specs/control.dart::ControlBlockEmitter |
| code_builder | ClosureExpression | specs/expression.dart::ExpressionVisitor::visitClosureExpression::expression |
| code_builder | LiteralSetExpression | specs/expression.dart::ExpressionVisitor::visitLiteralSetExpression::expression specs/expression.dart::literalSet specs/expression.dart::literalConstSet |
| code_builder | LiteralMapExpression | specs/expression.dart::ExpressionVisitor::visitLiteralMapExpression::expression specs/expression.dart::literalMap specs/expression.dart::literalConstMap |
| code_builder | LiteralRecordExpression | specs/expression.dart::ExpressionVisitor::visitLiteralRecordExpression::expression specs/expression.dart::literalRecord specs/expression.dart::literalConstRecord |
| code_builder | ControlBlock | specs/control.dart::ControlBlockVisitor::visitControlBlock::block specs/control.dart::LabeledControlBlock specs/control.dart::WhileLoop specs/control.dart::ForLoop specs/control.dart::ForInLoop specs/control.dart::CatchBlock |
| code_builder | LabeledControlBlock | specs/control.dart::ControlBlockVisitor::visitLabeledBlock::block specs/control.dart::WhileLoop specs/control.dart::ForLoop specs/control.dart::ForInLoop |
| code_builder | ControlTree | specs/control.dart::ControlBlockVisitor::visitControlTree::tree specs/control.dart::Conditional specs/control.dart::TryCatch |
| code_builder | ControlExpression | specs/control.dart::ControlBlockVisitor::visitControlExpression::expression specs/expression.dart::ControlExpression::doStatement specs/expression.dart::ControlExpression::tryStatement specs/expression.dart::ControlExpression::finallyStatement specs/expression.dart::ControlExpression::onStatement::statement |
| code_builder | Switch | specs/control.dart::ControlBlockVisitor::visitSwitch::statement specs/control.dart::SwitchStatement specs/control.dart::SwitchExpression |
| code_builder | Branch | specs/control.dart::BranchBuilder specs/control.dart::Branch |
This check can be disabled by tagging the PR with skip-leaking-check.
License Headers ✔️
// Copyright (c) 2025, the Dart project authors. Please see the AUTHORS file
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.
| Files |
|---|
| no missing headers |
All source files should start with a license header.
Unrelated files missing license headers
| Files |
|---|
| pkgs/bazel_worker/benchmark/benchmark.dart |
| pkgs/bazel_worker/example/client.dart |
| pkgs/bazel_worker/example/worker.dart |
| pkgs/benchmark_harness/integration_test/perf_benchmark_test.dart |
| pkgs/boolean_selector/example/example.dart |
| pkgs/clock/lib/clock.dart |
| pkgs/clock/lib/src/clock.dart |
| pkgs/clock/lib/src/default.dart |
| pkgs/clock/lib/src/stopwatch.dart |
| pkgs/clock/lib/src/utils.dart |
| pkgs/clock/test/clock_test.dart |
| pkgs/clock/test/default_test.dart |
| pkgs/clock/test/stopwatch_test.dart |
| pkgs/clock/test/utils.dart |
| pkgs/coverage/lib/src/coverage_options.dart |
| pkgs/html/example/main.dart |
| pkgs/html/lib/dom.dart |
| pkgs/html/lib/dom_parsing.dart |
| pkgs/html/lib/html_escape.dart |
| pkgs/html/lib/parser.dart |
| pkgs/html/lib/src/constants.dart |
| pkgs/html/lib/src/encoding_parser.dart |
| pkgs/html/lib/src/html_input_stream.dart |
| pkgs/html/lib/src/list_proxy.dart |
| pkgs/html/lib/src/query_selector.dart |
| pkgs/html/lib/src/token.dart |
| pkgs/html/lib/src/tokenizer.dart |
| pkgs/html/lib/src/treebuilder.dart |
| pkgs/html/lib/src/utils.dart |
| pkgs/html/test/dom_test.dart |
| pkgs/html/test/parser_feature_test.dart |
| pkgs/html/test/parser_test.dart |
| pkgs/html/test/query_selector_test.dart |
| pkgs/html/test/selectors/level1_baseline_test.dart |
| pkgs/html/test/selectors/level1_lib.dart |
| pkgs/html/test/selectors/selectors.dart |
| pkgs/html/test/support.dart |
| pkgs/html/test/tokenizer_test.dart |
| pkgs/html/test/trie_test.dart |
| pkgs/html/tool/generate_trie.dart |
| pkgs/pubspec_parse/test/git_uri_test.dart |
| pkgs/stack_trace/example/example.dart |
| pkgs/watcher/test/custom_watcher_factory_test.dart |
| pkgs/yaml_edit/example/example.dart |
- downgrade dev deps to work with sdk constraint
|
@jason-mayer there seem to be some file conflicts here. |
Background
The implementation is loosely based around discussions in #834. Several ideas were proposed in that thread, but it appears none have been implemented. The primary issue with control-flow implementation, as summed up by @matanlurey, is that:
This implementation works around that issue by using shared visitor logic, similar to the "compound statement builder" idea proposed by @eredo, taking advantage of the fact that all control-flow blocks in Dart are essentially formatted the same. Loosely, every control block can be broken down into:
Going off of that, we really only have to implement visitors for three logical "levels" of control blocks, those being:
ControlExpression, a control-flow expressionControlBlock, aControlExpressionfollowed by a bracketed bodyControlTree, a "tree" of multipleControlBlocks (ieif/else if/else)For (almost) every concrete implementation of a control-flow block, all it takes is then implementing
ControlBlockorControlTreeand the visiting logic comes free of charge.Control Expressions
Control Expressions are implemented via the internal-only
ControlExpressionclass, a newExpressionsubtype. The visitor logic for this class is the most complex due to the amount of variation in the formatting of control-flow expressions. It provides numerous constructors, covering each type of control-flow expression (if,for,while, etc.), and should (hopefully) be extendable enough to easily accommodate new control expression types if/when the language is updated.The class is internal-only, as all of its functionality is exposed via friendlier public builders. Making it public would only add scope noise and potential confusion.
Similar to the way
ExpressionVisitorandExpressionEmitterare structured, this addsControlBlockVisitorandControlBlockEmitteras abstract mixin classes mixed intoDartEmitter. Visitor logic forControlExpressionis in these control block mixins (rather than the expression ones) for maintainability, as it is easier to have everything in one place instead of arguably the most important part being in a separate file.Control Blocks
Control Blocks are implemented via the internal-only
ControlBlockmixin class. Rather than just exporting that, I decided on something similar to what @TekExplorer proposed, where each specific type of control block implements a public "helper" builder on top ofControlBlockto make the API more user-friendly. In total, there are 5 of these builders:ForLoop(for)ForInLoop(for-in,await for)WhileLoop* (while,do-while)(see conversation)Condition(if,else if,else) (single)if-caseviaConditionBuilder.ifCaseBranchBuilder.ifCaseCatchBlock(catch)WhileLooprequires a custom visitor due to the existence of thedo-whileloop, which does not follow the "standard" control block format. All others simply mix inControlBlock.I opted not to support single-line control blocks (with no brackets). These are entirely stylistic; functionally it does not matter whether
if (value) return;has brackets or not, andif (value) return;is really the only place where this would be used, as it is bad form to omit the braces if the statement body overflows onto the next line. That being said, I did addExpression.ifThenReturnwhich is shorthand to generate such an expression and which, out of simplicity, omits the braces (see Expression Helpers).You may have noticed that I didn't include
switchstatements in this section. No surprise, they also required a separate visitor, among other unique implementation details, so they are in their own section, see Switches.Control Trees
Control Trees are implemented via the internal-only
ControlTreemixin class, using the same helper builder concept as control blocks. In total there are two such builders:IfTreeConditionalfor creatingif-elsetrees (see conversation)TryCatchfortry/catch/finallytrees.(see conversation)IfTreewas named that way instead of something likeConditionalTreein order to avoid autocomplete conflicts withConditionwhen using the API, which proved to be sufficiently annoying during development.As
tryandfinallyblocks don't have any parameters,TryCatchimplements them as aBlockfor simplicity, hence the lack of aTryBlockorFinallyBlock.CatchBlockis not implemented this way to support customizing the error/stacktrace parameter names as well asonclauses.Switches
The implementation for
switchstatements and expressions is fairly complex, largely due to the existence of two differentswitchtypes. Because of this, most functionality is located in the internal-onlySwitchandSwitchBuilderclasses, which bothswitchsubtypes (SwitchStatementandSwitchExpression) then extend.Rather than confusingly exposing two
Casetypes (switchis bad enough), genericCase<T>is public, with theswitch-specific implementations being in two separate internal classes,CaseStatementandCaseExpression.Case<T>is not visitable; the aforementioned specific implementations each implementCodeand have their own visitors.The two
switchtypes each provide a private getter_cases, which converts theCase<T>s from theswitch's publiccasesfield into their visitable,switch-specific counterparts. When visited, theswitch'sControlExpressionis generated, and_casesis used to create aBlock. These two values are then passed into aBuildableSwitch, yet another internal-only class, which implementsControlBlock.After all that (admittedly convoluted) conversion, the
BuildableSwitchis passed into the control block visitor and from then on generated the same way as any otherControlBlock.Expression Helpers
Several helper methods were also added to
Expressionto facilitate easier usage of control-flow blocks. Rather than adding them toExpressiondirectly, they were added toControlFlow, an extension onExpression. This was done primarily for ease of maintenance due to the unwieldy nature ofexpression.dart, but also to allow users of the package to easily be able opt-out of these specific helpers viahideif desired.Added the following helpers:
Expression.yielded,Expression.yieldStarredExpression.ifThenExpression.ifThenReturnExpression.loopWhile,Expression.loopDoWhileExpression.loopForInAdditionally added some static helpers to
ControlFlow:ControlFlow.breakVoid,ControlFlow.breakLabelControlFlow.continueVoid,ControlFlow.continueLabelControlFlow.returnVoidControlFlow.rethrowVoidControlFlow.ifCaseFinally, static
Expression.wildcardwas also added. It was added toExpressionrather thanControlFlowas it felt more idiomatic (a wildcard isn't really a control flow statement like the others onControlFloware)Collection Control-Flow
Collection control-flow support was implemented as static methods on the
ControlFlowextension, particularly:ControlFlow.collectionIfControlFlow.collectionElseControlFlow.collectionForControlFlow.collectionForInThese are fairly simple; however, they did require some slight changes to
visitAlland the various collection literal visitors in order to support chaining, ie:I tried to implement this via a child/nesting system, but that ended up becoming prohibitively complicated (primarily due to map literals). A chaining-based system proved to be much simpler.
Other Changes
.g.dartfiles were generated with an older version ofbuilt_valueand contained some redundant checks andnewkeyword usage. When I ran the built script it updated all those files, so I've also pushed those versions.expression_test.dartfile and on the changelog (also, apologies, I guess my editor automatically formatted the changelog and replaced all-with*, something which I didn't realize until just now as I am writing this)4.10.2-wipto4.11.0-wipinpubspec.yamlTests
Added comprehensive unit tests for all changes. Per
package:coverageand LCOV, all touched files (excluding generated.g.dart) have at least 97% test coverage.Happy to answer any questions or make any changes requested, I've got lots of free time at the moment and this is something I'd love to see
code_buildersupport.Contribution guidelines:
dart format.Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.